Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ptime SDP element to accept float type values #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mitchell-Roy
Copy link

No description provided.

@Mitchell-Roy
Copy link
Author

@ibc Here is a proposed change for ptime element in sdp. Thanks!

@ibc
Copy link
Owner

ibc commented Mar 31, 2020

Thanks :)

BTW have you run tests? may be you could modify some existing test SDP by adding a ptime with float value and then check in in parse.test.cpp?

@Mitchell-Roy
Copy link
Author

Yes, haven't got to that yet, will try to add this in as soon as I get a chance :)

@ibc
Copy link
Owner

ibc commented Mar 31, 2020

Note tha Travis CI is failing due to this change: https://travis-ci.com/github/ibc/libsdptransform/jobs/310698468#L691

Somehow I expected this. The problem is that this PR forces ptime to be printed as float even if it was an integer. Example: a=ptime:20.0.

IMHO it's critical that we represent it as an integer ( a=ptime:20) if it does not have decimals. I'm not sure all RTP stacks will like ptime with a float number so let's avoid float representation when it's not needed. I don't know right now how to do that.

BTW: why this change?

- "ptime:%d"
+ "ptime:%s"

I understand it cannot be %d if it contains a float, but why %s?

@Mitchell-Roy
Copy link
Author

Mitchell-Roy commented Mar 31, 2020

Yeah, I noticed the writer will force a decimal place as well. I wanted to make it something similar to the "framerate" element as it handles similar inputs so I used that as a point of reference..

And yes, ideally we'd want to not have a floating point representation if not needed. I'm not quite sure how one would go about ensuring that atm :)

@ibc
Copy link
Owner

ibc commented Mar 31, 2020

You are right, sorry. Maybe the same issue happens in a=framerate. In fact, the tests just have decimal framerates, that's why it works :)

@ibc
Copy link
Owner

ibc commented Mar 31, 2020

Note that, instead of format you can define a formatFunc (there are some attributes in grammar.cpp using that).

@ibc
Copy link
Owner

ibc commented Mar 31, 2020

In such a formatFunc you may use some helper to check if it's a float or just an integer. In parser.cpp there we already use:

#include <sstream>   // std::istringstream
#include <ios>       // std::noskipws

bool isFloat(const std::string& str)
{
	std::istringstream iss(str);
	float f;

	iss >> std::noskipws >> f;

	return iss.eof() && !iss.fail();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants